Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add task solution #783

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

add task solution #783

wants to merge 12 commits into from

Conversation

bashtovaA
Copy link

@bashtovaA bashtovaA commented Oct 4, 2023

Copy link

@IvanFesenko IvanFesenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, keep going. Can't find issue with background

@bashtovaA bashtovaA requested a review from IvanFesenko October 5, 2023 16:05
Copy link

@loralevitska loralevitska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep up the good work! Request next review after finish all parts of landing
Some comments to fix:

  • fix slider according design
    image
  • make sure that all interactive elements have a cursor pointer on hover
  • fix menu, it doesn't work

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replied about header in chat. Waiting other sections.

Copy link

@andriy-shymkiv andriy-shymkiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • it shouldn't be this black corners here:
    image

  • this img goes beyong this container:
    image

  • this images is not centered properly:
    image

  • contact us section is missing 😞

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost done.
Needs some improvements.

  • hover effects for these cards looks unexpected. is it should have such behavior?
    chrome-capture-2023-10-10
  • add transition for hover on social icon and add hover (change color) for email, address, phone links
    Screenshot from 2023-11-10 17-42-30
  • center apply button. why do you need to make it with position absolut on some widht and hardcode margins? it can be with normal document flow (one element below other).
    margin: 0 auto can center button horizontally.
    Screenshot from 2023-11-10 17-42-19
    Screenshot from 2023-11-10 17-44-30
  • check and comare carefully design with your implementation. For example spaces between cards, centering horizontally for tablet and mobild and so on.
    actual:
    Screenshot from 2023-11-10 17-46-21
    chrome-capture-2023-10-10

design:
Screenshot from 2023-11-10 17-46-12

  • after click on burger icon nothing happens. menu is not open.

Copy link

@DarkMistyRoom DarkMistyRoom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The demo link doesn't work
image

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Keep doing:
Screenshot 2023-11-13 at 12 57 16
Screenshot 2023-11-13 at 12 57 33
Screenshot 2023-11-13 at 12 57 45
Screenshot 2023-11-13 at 12 58 04
Consider fixing validation errors
Screenshot 2023-11-13 at 12 55 06
Consider disabling scroll under the menu and enabling the menu scroll
Screenshot 2023-11-13 at 12 54 25 Screenshot 2023-11-13 at 13 00 42
Add a proper href attribute here
Screenshot 2023-11-13 at 12 54 15
The button should be of type submit and should be inside the form
Screenshot 2023-11-13 at 12 53 17
Screenshot 2023-11-13 at 12 53 24
Screenshot 2023-11-13 at 12 53 37
Screenshot 2023-11-13 at 12 53 45
Some sections on mobiles are shifted
Screenshot 2023-11-13 at 12 53 00
This link leads to 404 page, consider fixing it

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Need to make all these fields are required before sending and need to check width for the inputs
image
  1. These elements should be on the center horizontally
image
  1. Also, all content on the page must have a maximum width and must be centered on the page horizontally, and now it stretches to the full width of the browser window

  2. You need to hide the text below the menu

image
  1. Need to fix on the large screens
image

Copy link

@IvanFesenko IvanFesenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants